-
Notifications
You must be signed in to change notification settings - Fork 20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
An improved workflow for maintaining Salt #96
base: master
Are you sure you want to change the base?
Conversation
00367fd
to
a8616ca
Compare
|
||
As mentioned this is now at `pkg/suse/salt.changes` in `openSUSE/salt` GitHub repo. | ||
|
||
When creating a PR to `openSUSE/salt` the user must also include the corresponding changes to the spec file, that can be generated as usual with `osc vc`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if I get this point right. Maybe it says spec file
but the actual meaning is different, not sure, could you please clarify it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woops, sorry. I meant "changelog" file 😄
Let me fix this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do I understand it right, that in this case we will add changelog entries manually to the changelog file or manually but with osc vc
, still not fully clear here. With the osc services there is way to use commit messages as a changlog items, don't we want to use it this way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm proposing here to manually add the changelog entry to salt.changes
by using the osc vc
command directly in your Git tree, so the generated changelog entry (and header) can be included together with the code changes in the PR to openSUSE/salt
repo.
This way a single PR to openSUSE/salt
repo could contain all possible different bits: code changes, specfile changes, changelog entries, artifacts changes.
See this example PR: https://github.com/meaksh/salt/pull/10/files
Now that you mentioned the "osc services" for the changelog, I realized that the current proposal is not covering the fact that we need to maintain different changelogs depending on the targeted codestreams (as they are not aligned).
I'll have some thoughts and clarify this on the RFC text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I generally like this design. I left a few questions in line but saved the big one for now: How do we package the Salt Bundle? Currently we have a split-brain problem where some of the sources are just in OBS. Can we integrate them into this workflow?
|
||
As mentioned this is now at `pkg/suse/salt.changes` in `openSUSE/salt` GitHub repo. | ||
|
||
When creating a PR to `openSUSE/salt` the user must also include the corresponding changes to the changes file, that can be generated as usual with `osc vc`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this work for the different changelogs we currently maintain in parallel?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed to cover on the current proposal our requirement on maintaining different changelogs for the different target codestream we maintain.
I'll add some more text to cover these cases.
|
||
When creating a PR to `openSUSE/salt` the user must also include the corresponding changes to the changes file, that can be generated as usual with `osc vc`. | ||
|
||
Similarly to the main Uyuni repository, we should add a GitHub action to warn the user in case no changelog entry is added in the PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we also merging the changelog like we do in Uyuni or do we need to resolve merge conflicts manually?
|
||
#### `systemsmanagement:saltstack/salt` | ||
|
||
This OBS package is a branch from `systemsmanagement:saltstack:github/salt`, where we disable the services and manually run them to get the spec file, changelog and obsinfo/obscpio files, so the package can be submitted to openSUSE or SLE. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
branch
is maybe not the best term, it suggest a temporary package that's submitted back after changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll rephrase this
|
||
#### `systemsmanagement:saltstack:github/salt` | ||
|
||
This OBS package will only contain `_multibuild` file and a `_service` file that should look like: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have the _multibuild
in git as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, indeed probably yes. I'll check and adjust this.
* package building according to PR branch. | ||
* branched and removed automatically from `systemsmanagement:saltstack:github/salt` by OBS workflow. | ||
|
||
The same OBS structure will apply to all our OBS targets: `products`, `products:testing` and `products:next`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain how the structure applies to these projects concretely? I would have thought products:testing
or products:next
don't need a separate github
subproject.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, actually for products
we don't really needed, as we just copypac whatever is in products:testing
to products
.
But for products:testing
and products:next
I will also consider the github
structure, to be able to have different Salt versions if necessary ensuring those packages are also ready to be consumed (even if those are never be directly released) but we would prevent enabled services can run unexpectely on targets that are linked to products:testing
and products:next
(like i.a. Uyuni:Master or D:G:M:*)
|
||
### Making OBS packages ready to be submitted to Maintenance | ||
|
||
Since the package at `systemsmanagement:saltstack:github/salt` has "services" enabled, and we cannot enable/disable services using OBS workflows, this means this package is not yet ready to be submitted to openSUSE or SLE, as they don't accept enabled services on their submissions. We must disable the services. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me it sounds like a conceptual problem using services to sync the sources. An alternative we could at least look at is using src.opensuse.org as the git forge (according to https://openbuildservice.org/help/manuals/obs-user-guide/cha-obs-scm-bridge#sec-obs-obs-scm-bridge-update-notifications this does not require a _service to sync the sources)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, interesting. I'll give it a try 👍
<param name="scm">git</param> | ||
<param name="versionformat">@PARENT_TAG@</param> | ||
<param name="versionrewrite-pattern">v(.*)</param> | ||
<param name="revision">openSUSE/devel/master</param> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was not aware we used an openSUSE/devel/master
branch. Is this a new branch we will use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used openSUSE/devel/master
here just as an example, it should point to the eventual openSUSE/release/3008.x
branch. I'll fix this on the RFC text.
Currently openSUSE/devel/master
is just the devel branch I created with upstream master
branch + our patches partially rebased on top (excluding patches to extensions).
|
||
### Salt Extensions | ||
|
||
#### Builtin extensions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been part of the other RFC, but for me it's still not so clear that we can have "builtin extensions". How do we publish these to PyPI from the main repository? How do we get them to show up on https://extensions.saltproject.io/?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me, just a few questions to help me better understand the solution
All current extra "Sources" files in our RPM package, together with spec file and changelog file will go now to a `pkg/suse/` directory in `openSUSE/salt`: | ||
|
||
``` | ||
pkg/suse/README.SUSE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to make is clear, any patch that we want to apply on top of salt code base should go to this folder pkg/suse/
right?
So basically you are moving all the content from the github project openSUSE/salt-packaging
(subfolder salt) to this noew pkg/suse
folder (at the start we will not have patches since we start from the same base as upstream)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really. We won't be carrying patch files anymore in our new OBS packages, as any code change will automatically inside the source tarball by the obs_scm
integration.
The exception to this would be EMBARGOED bugs, where we cannot proceed publicly via GitHub, so we will put a patch file manually in IBS than will be removed the bug is public and we push the changes to GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A side question; currently, we have a lot of commits that are not in upstream, due to various reasons. With salt-packaging
, we can easily check which patches are in upstream, and which patches are not. With removing the patch workflow, we're losing this insight.
Do you perceive this as a problem? Are we still sticking with the "fork & cherrypick commits" development style, or are we moving towards rebasing more frequently?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good point! We should probably want to have a way to easily identify what is upstreamed and what is not.
I'll elaborate this a bit on the RFC text. Thanks for the note 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you @m-czernek , that was exactly my concern when I made the comment. Commits that are not yet upstrem, and currently are maintained on salt-packaging project.
We we start merging commit to our salt project that are not merge upstream yet, it can make it harder to integrate upstream version and know what is merge already or not.
@meaksh thank you for look into this topic.
Read the RFC here